Skip to content

[SPARK-12934][SQL] Count-min sketch serialization#10893

Closed
liancheng wants to merge 5 commits intoapache:masterfrom
liancheng:cms-serialization
Closed

[SPARK-12934][SQL] Count-min sketch serialization#10893
liancheng wants to merge 5 commits intoapache:masterfrom
liancheng:cms-serialization

Conversation

@liancheng
Copy link
Contributor

@liancheng liancheng commented Jan 25, 2016

This PR adds serialization support for CountMinSketch.

A version number is added to version the serialized binary format.

@SparkQA
Copy link

SparkQA commented Jan 25, 2016

Test build #49967 has finished for PR 10893 at commit e97d7f9.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • throw new CountMinSketchMergeException(\"Cannot merge estimator of class \" + other.getClass().getName());
    • public class CountMinSketchMergeException extends Exception

rxin
rxin reviewed Jan 25, 2016
View reviewed changes
common/sketch/src/main/java/org/apache/spark/util/sketch/CountMinSketch.java Outdated
Copy link
Contributor

@rxin rxin Jan 25, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's put the implementation in CountMinSketchImpl, and simply delegate to a static method there

Copy link
Contributor

@rxin rxin Jan 25, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also somewhere in CountMinSketchImpl we should document the serialization format.

@liancheng liancheng mentioned this pull request Jan 25, 2016
Closed
cloud-fan
cloud-fan reviewed Jan 25, 2016
View reviewed changes
common/sketch/src/main/java/org/apache/spark/util/sketch/CountMinSketch.java
Copy link
Contributor

@cloud-fan cloud-fan Jan 25, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move this out so that we can also use it in bloom filter?

Copy link
Contributor

@rxin rxin Jan 25, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be count min sketch specific, because the two binary protocols can and should evolve separately.

@SparkQA
Copy link

SparkQA commented Jan 25, 2016

Test build #50023 has finished for PR 10893 at commit 9acfe33.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@liancheng liancheng mentioned this pull request Jan 25, 2016
Closed
@SparkQA
Copy link

SparkQA commented Jan 25, 2016

Test build #50025 has finished for PR 10893 at commit 4636af1.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Jan 25, 2016

Test build #50020 has finished for PR 10893 at commit 4abc4e0.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

cloud-fan
cloud-fan reviewed Jan 25, 2016
View reviewed changes
common/sketch/src/main/java/org/apache/spark/util/sketch/CountMinSketchImpl.java
Copy link
Contributor

@cloud-fan cloud-fan Jan 25, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we move this comment near writeTo?

Copy link
Contributor Author

@liancheng liancheng Jan 25, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I couldn't decide whether to put it before writeTo or readFrom, and then ended up here...

@cloud-fan
Copy link
Contributor

cloud-fan commented Jan 25, 2016

LGTM overall

rxin
rxin reviewed Jan 25, 2016
View reviewed changes
common/sketch/src/main/java/org/apache/spark/util/sketch/CountMinSketchImpl.java
Copy link
Contributor

@rxin rxin Jan 25, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should add some check here to throw an exception if the version number is not 1.

@rxin
Copy link
Contributor

rxin commented Jan 25, 2016

Going to merge this. Thanks. Please address my (one) feedback in your next pr.

@asfgit asfgit closed this in 6f0f1d9 Jan 25, 2016
@liancheng liancheng deleted the cms-serialization branch January 25, 2016 23:09
tedyu
tedyu reviewed Jan 25, 2016
View reviewed changes
common/sketch/src/main/java/org/apache/spark/util/sketch/CountMinSketchImpl.java
Copy link
Contributor

@tedyu tedyu Jan 25, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be closed before returning from the method, right ?

Copy link
Contributor Author

@liancheng liancheng Jan 26, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But DataInputStream.close() also closes the underlying input stream, which isn't our expected behavior, is it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@liancheng @SparkQA @cloud-fan @rxin @tedyu @yhuai